You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns
Credential Exposure: The code handles user credentials for authentication. While the implementation appears to use proper credential objects, care should be taken to ensure credentials are not logged or exposed, especially with the logging handler added on line 254. The credentials should also be properly secured in memory.
The HttpClient and HttpClientHandler instances should be properly disposed. Consider implementing IDisposable pattern or ensuring proper cleanup in the class.
protectedvirtualHttpClientHandlerCreateHttpClientHandler(){HttpClientHandlerhttpClientHandler=newHttpClientHandler();stringuserInfo=this.remoteServerUri.UserInfo;if(!string.IsNullOrEmpty(userInfo)&&userInfo.Contains(":")){string[]userInfoComponents=this.remoteServerUri.UserInfo.Split(newchar[]{':'},2);httpClientHandler.Credentials=newNetworkCredential(userInfoComponents[0],userInfoComponents[1]);httpClientHandler.PreAuthenticate=true;}httpClientHandler.Proxy=this.Proxy;returnhttpClientHandler;}/// <summary>/// Creates an instance of <see cref="HttpClient"/> used by making all HTTP calls to remote end./// </summary>/// <returns>An instance of <see cref="HttpClient"/>.</returns>protectedvirtualHttpClientCreateHttpClient(){varhttpClientHandler=CreateHttpClientHandler();HttpMessageHandlerhandler=httpClientHandler;
✅ Add null check for handlerSuggestion Impact:The commit implements a null check for httpClientHandler using null-coalescing operator with throw, which achieves the same goal as the suggested if-statement approach
code diff:
- var httpClientHandler = CreateHttpClientHandler();+ var httpClientHandler = CreateHttpClientHandler() ?? throw new InvalidOperationException($"{nameof(CreateHttpClientHandler)} method returned null");
Add null check for the returned HttpClientHandler from CreateHttpClientHandler() before using it to avoid potential NullReferenceException
protected virtual HttpClient CreateHttpClient()
{
var httpClientHandler = CreateHttpClientHandler();
+ if (httpClientHandler == null)+ {+ throw new InvalidOperationException("CreateHttpClientHandler() returned null");+ }
HttpMessageHandler handler = httpClientHandler;
Apply this suggestion
Suggestion importance[1-10]: 8
__
Why: The suggestion addresses a potential null reference exception that could occur if CreateHttpClientHandler() returns null. This is a critical defensive programming practice for virtual methods that can be overridden by derived classes.
Medium
Learned best practice
✅ Add null validation check for required class field to prevent potential NullReferenceExceptionSuggestion Impact:While the exact suggested null check wasn't added, the commit added a null check in CreateHttpClient() that validates the result of CreateHttpClientHandler(), which would catch null remoteServerUri issues
code diff:
+ var httpClientHandler = CreateHttpClientHandler()+ ?? throw new InvalidOperationException($"{nameof(CreateHttpClientHandler)} method returned null");
The CreateHttpClientHandler() method uses this.remoteServerUri without validating that it's not null. Since this is a required field for the class functionality, add a null check at the start of the method to fail fast with a clear error message.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Motivation and Context
Implements #15280
Types of changes
Checklist
PR Type
Enhancement
Description
Introduced the ability to override
HttpClientHandlerfor all HTTP requests.Added a new protected method
CreateHttpClientHandlerfor custom handler creation.Enhanced
CreateHttpClientto utilize the newCreateHttpClientHandlermethod.Improved flexibility for HTTP client customization in remote WebDriver interactions.
Changes walkthrough 📝
HttpCommandExecutor.cs
Added support for customizable HttpClientHandlerdotnet/src/webdriver/Remote/HttpCommandExecutor.cs
CreateHttpClientHandlerfor creatingcustom
HttpClientHandler.CreateHttpClientto use the newCreateHttpClientHandlermethod.
CreateHttpClientandCreateHttpClientHandler.